fix: restrict SentinelResourceAspect pointcut to execution join points - #3606
fix: restrict SentinelResourceAspect pointcut to execution join points#3606daguimu wants to merge 1 commit into
Conversation
…s only The @annotation pointcut without an explicit join point kind matches both call and execution join points in native AspectJ (compile-time or load-time weaving). This causes the around advice to fire twice for the same method invocation on the same thread, doubling the thread count tracked by Sentinel and leading to incorrect thread-based flow control. Add explicit execution(* *(..)) to the pointcut expression to restrict matching to execution join points only. This has no effect on Spring AOP (which already only supports execution join points) but fixes the double-counting issue in native AspectJ environments. Fixes alibaba#3597
oss-sentinel-ai
left a comment
oss-sentinel-ai
left a comment
There was a problem hiding this comment.
Summary
Fixes double advice firing under native AspectJ (CTW/LTW): the unqualified @annotation(...) pointcut matches both call and execution join points, so @SentinelResource methods were entered twice (SphU.entry() called twice), corrupting thread-count based flow control. Restricting the pointcut to execution(* *(..)) restores single-firing semantics and is a no-op for Spring AOP users, since proxy-based AOP only supports execution join points. The pointcut is referenced by the single @Around advice in this class, so the change is fully contained. Well-analyzed issue and correct one-line fix.
Findings
- [Info] SentinelResourceAspect.java:38 — optional: a load-time-weaving regression test if feasible.
LGTM.
Automated review by github-manager-bot
| public class SentinelResourceAspect extends AbstractSentinelAspectSupport { | ||
|
|
||
| @Pointcut("@annotation(com.alibaba.csp.sentinel.annotation.SentinelResource)") | ||
| @Pointcut("execution(* *(..)) && @annotation(com.alibaba.csp.sentinel.annotation.SentinelResource)") |
There was a problem hiding this comment.
Correct fix. In native AspectJ (CTW/LTW), an unqualified @annotation(...) pointcut matches multiple join point kinds — including both call and execution — so the advice fires twice per invocation, double-counting SphU.entry() and breaking thread-based flow control. Restricting to execution(* *(..)) restores the intended single-firing semantics while remaining a no-op for Spring AOP users (proxy-based AOP only supports execution join points anyway).
Optional follow-up (non-blocking): if feasible, a regression test exercising the aspect under load-time weaving would guard against regressions, though that may be non-trivial to wire into the current test setup.
2 participants
Problem
When using native AspectJ (compile-time or load-time weaving) instead of Spring AOP, the
@SentinelResourceannotation aspect fires twice for the same method call on the same thread — once at thecalljoin point and once at theexecutionjoin point. This causesSphU.entry()to be called twice, doubling the thread count tracked by Sentinel and resulting in incorrect thread-based flow control (e.g., a thread limit of 2 effectively behaves as a limit of 1).Root Cause
The current pointcut
@annotation(com.alibaba.csp.sentinel.annotation.SentinelResource)does not specify a join point kind. In Spring AOP (proxy-based), this only matchesexecutionjoin points since that's all Spring AOP supports. However, in native AspectJ,@annotationmatches multiple join point kinds including bothcallandexecution, causing the advice to trigger twice per method invocation.Fix
Changed the pointcut from:
to:
This explicitly restricts matching to
executionjoin points only, which:Tests
All 16 existing tests in
sentinel-annotation-aspectjpass, includingSentinelAnnotationIntegrationTestwhich validates the annotation-based flow control with Spring AOP.Impact
Only affects
SentinelResourceAspect.javapointcut expression. No behavioral change for Spring AOP users. Fixes thread counting accuracy for native AspectJ users.Fixes #3597